Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed bug unpacking Overlay and Layout objects in Layout.from_values #1088

Merged
merged 14 commits into from
Feb 23, 2017

Conversation

philippjfr
Copy link
Member

Fix for #1081, passing Layouts and Overlays to their respective constructors correctly unpacks them again after regression caused them to be nested.

elements = [hv.Curve(np.random.rand(10)) * hv.HLine(0) for i in range(4)]
overlay = hv.Overlay(elements)
print(repr(overlay))

This now correctly produces an object with this repr:

:Overlay
   .Curve.I   :Curve   [x]   (y)
   .HLine.I   :HLine   [x,y]
   .Curve.II  :Curve   [x]   (y)
   .HLine.III :HLine   [x,y]
   .Curve.III :Curve   [x]   (y)
   .HLine.IV  :HLine   [x,y]
   .Curve.IV  :Curve   [x]   (y)
   .HLine.V   :HLine   [x,y]

@philippjfr
Copy link
Member Author

Just noticed that it's skipping HLine.II instead adding HLine.V this was also the case before the regression but it seems like a bug.

@philippjfr
Copy link
Member Author

With my latest fix the repr now looks correct.

:Overlay
   .Curve.I   :Curve   [x]   (y)
   .HLine.I   :HLine   [x,y]
   .Curve.II  :Curve   [x]   (y)
   .HLine.II  :HLine   [x,y]
   .Curve.III :Curve   [x]   (y)
   .HLine.III :HLine   [x,y]
   .Curve.IV  :Curve   [x]   (y)
   .HLine.IV  :HLine   [x,y]

Going to need a lot of unit tests.

@philippjfr philippjfr added the type: bug Something isn't correct or isn't working label Jan 28, 2017
@jlstevens
Copy link
Contributor

jlstevens commented Jan 29, 2017

Looks good but I will wait to see examples of what the _unpack_paths classmethod does in the unit tests before carrying out a more detailed review.

@philippjfr
Copy link
Member Author

There's definitely still things going wrong. Will have to look into this more.

@philippjfr
Copy link
Member Author

philippjfr commented Feb 10, 2017

Finally managed to resolve this, still some unit tests to add but the current notebook test failure in Showcase is real:

freq1 = hv.Image(sine(grid, freq=50))  + hv.Curve(zip(dist, sine(dist**2, freq=50)))
freq2 = hv.Image(sine(grid, freq=200)) + hv.Curve(zip(dist, sine(dist**2, freq=200)))
(freq1 + freq2).cols(2)

With the PR

:Layout
   .Image.I  :Image   [x,y]   (z)
   .Curve.I  :Curve   [x]   (y)
   .Image.II :Image   [x,y]   (z)
   .Curve.II :Curve   [x]   (y)

Without the PR:

:Layout
   .Image.I   :Image   [x,y]   (z)
   .Curve.I   :Curve   [x]   (y)
   .Image.II  :Image   [x,y]   (z)
   .Curve.III :Curve   [x]   (y)

Hopefully this is the only thing that snuck in while our tests weren't running properly.

@jlstevens
Copy link
Contributor

jlstevens commented Feb 10, 2017

I agree the new repr looks correct. Happy to merge once the tests are updated and passing.

Copy link
Contributor

@jlstevens jlstevens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would still be nice to have examples of what the _unpack_paths classmethod does in unit tests. Otherwise, I am happy to merge once the notebook tests are passing.

@philippjfr
Copy link
Member Author

So I just checked out some old versions and it turns out the issue above, which is now been fixed, has always been there.

@philippjfr
Copy link
Member Author

philippjfr commented Feb 11, 2017

So this code turned out to be quite a mess, the functionality was duplicated across two methods and spread out even further into various __mul__ and __add__ methods. The new approach is a) more correct (as evidenced by the bug mentioned above) and b) more consistent as paths are always recomputed in the same way rather than relying on brittle code to rename existing paths.

The new algorithm can be summed up in two steps:

  1. Find clashing path names.
  2. Iterate over values and if a path (computed from group/label) is not unique append a roman numeral corresponding to the current count for the non-unique path.

Previously we had a separate method which would merge existing Overlay items by trying to make their paths unique. This approach did not really work (again note the example above), and made the code more complex. The issue is that merging existing paths cannot be done correctly without trying to figure out which parts of the existing paths are roman numerals which is bound to be brittle. In effect the results should either be more correct or the same (as evidenced by most of the tests passing), unless you construct Layouts or Overlays with custom paths, which is presumably fairly rare. We do use custom paths in featuremapper/topographica but in those cases we use a Layout as a storage container and don't generally expect to merge them by using +.

@philippjfr philippjfr force-pushed the overlay_fix branch 4 times, most recently from 8084606 to fba8d29 Compare February 12, 2017 03:12
@jlstevens
Copy link
Contributor

unless you construct Layouts or Overlays with custom paths, which is presumably fairly rare

I'm not sure that is a safe assumption. Could you give a simple example of how it was supposed to work before, how it actually worked (assuming it was buggy) and how it works now?

My worry is that these changes could affect a lot of things and I want to understand the implications of this PR for backwards compatibility.

@philippjfr
Copy link
Member Author

When inheriting and merging custom paths in the previous implementation it would follow the following rule, cut the path down to length 2, if the second part of the path matches the label keep it, otherwise drop it. So the difference basically comes down to whether the first part of the path, corresponding to the object's group, is inherited or not. I can probably still make that work now that I've factored things out into smaller functions and can make sense of it.

@philippjfr
Copy link
Member Author

Okay I've reimplemented path inheritance which turned out to be much easier now that the code is factored into smaller chunks, some of which may be better as utilities. The behavior should now be the same as before except 1) numbering is correct and 2) it doesn't needlessly create a whole bunch of intermediate objects.

@jlstevens
Copy link
Contributor

Looks good!

I'm happy to merge once the tests pass. I would also check that the new underscore methods don't show up when tab-completing on Layouts...

@philippjfr
Copy link
Member Author

Still missing a lot of unit tests, perhaps I'll also make some of the underscore methods into utilities. Doubt they'll show up as we already had a bunch of underscored methods before.

@jlstevens
Copy link
Contributor

perhaps I'll also make some of the underscore methods into utilities.

Up to you! Might make good sense although if you have a bunch or related utilities, it might be worth defining them as classmethods on a utility class.

@philippjfr
Copy link
Member Author

@jlstevens Finally got around to adding some unit tests here, previous unit tests were only testing all the simple cases, which is how we missed this in the past. Ready to merge now when tests pass.

@jlstevens
Copy link
Contributor

Looks good!

This PR took a while but I am glad this issue is now fixed. The code looks easier to understand than before with the bonus of actually being more correct.

Merging.

@jlstevens jlstevens merged commit 9f777d3 into master Feb 23, 2017
@philippjfr philippjfr deleted the overlay_fix branch February 23, 2017 18:43
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: bug Something isn't correct or isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants